[SPARK-53573][SQL] IDENTIFIER everywhere #52765

srielau · 2025-10-28T18:24:13Z

What changes were proposed in this pull request?

We propose expanding the IDENTIFIER() clause, which turns a string into a qualified identifier, to all places identifiers can appear. The current clause is severely limited in where it can go because it accepts constant expressions, including session variables.
Due to the complexity of the argument the existing clause requires tricky code to incrementally analyze its arguments and then execute sections of parser code at a later point.
By contrast the generalized IDENTIFIER clause only allows string literals which can be processed in the visitor methods.
Due to the rework of parameter markers and string coalescing this allows for constructs such as:

SELECT * FROM IDENTIFIER(:cat '.' :schema '.' :table)

it even allows:

SELECT 'hello' AS IDENTIFIER(:alias);

This is really all identifier() needs. We may be able to deprecate and de-support the existing too complex identifier() implementation.

Why are the changes needed?

IDENTIFIER() is a popular feature, but it can only be used in very specific, hard to reason about places.
The new implementation preserved 99% of teh fucntion while expanding its use to everywhere.

Does this PR introduce any user-facing change?

Yes, it's a new feature

How was this patch tested?

expanded Parameters and inentifier-clause test suites.

Was this patch authored or co-authored using generative AI tooling?

Yes, Clause Sonnet 4.5

… maintainability - Extract IDENTIFIER_PREFIX constant for magic string - Improve getMultipartIdentifierText documentation with complete Scaladoc - Narrow exception handling from Exception to ParseException - Remove redundant ParserUtils prefix in class methods - Fix qualified label validation to check resolved identifiers - Ensure all comments are complete sentences ending with periods - Remove dead code and improve variable naming - Fix FOR loop variable resolution to use getMultipartIdentifierText All tests pass: - SqlScriptingParserSuite (qualified label validation) - SqlScriptingE2eSuite (identifier tests) - SQLQueryTestSuite (identifier-clause and identifier-clause-legacy)

move imports to head

dtenedor

Did an initial round of review. Thanks for working on this!

common/utils/src/main/resources/error/error-conditions.json

sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

sql/core/src/test/resources/sql-tests/inputs/identifier-clause-legacy.sql

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala

sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4

sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

sql/core/src/test/scala/org/apache/spark/sql/ParametersSuite.scala

… for loop label to strictIdentifier

sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

cloud-fan · 2025-11-13T03:46:56Z

thanks, merging to master/4.1!

### What changes were proposed in this pull request? We propose expanding the IDENTIFIER() clause, which turns a string into a qualified identifier, to all places identifiers can appear. The current clause is severely limited in where it can go because it accepts constant expressions, including session variables. Due to the complexity of the argument the existing clause requires tricky code to incrementally analyze its arguments and then execute sections of parser code at a later point. By contrast the generalized IDENTIFIER clause only allows string literals which can be processed in the visitor methods. Due to the rework of parameter markers and string coalescing this allows for constructs such as: ``` SELECT * FROM IDENTIFIER(:cat '.' :schema '.' :table) ``` it even allows: ``` SELECT 'hello' AS IDENTIFIER(:alias); ``` This is really all identifier() needs. We may be able to deprecate and de-support the existing too complex identifier() implementation. ### Why are the changes needed? IDENTIFIER() is a popular feature, but it can only be used in very specific, hard to reason about places. The new implementation preserved 99% of teh fucntion while expanding its use to everywhere. ### Does this PR introduce _any_ user-facing change? Yes, it's a new feature ### How was this patch tested? expanded Parameters and inentifier-clause test suites. ### Was this patch authored or co-authored using generative AI tooling? Yes, Clause Sonnet 4.5 Closes #52765 from srielau/identifier-lite. Authored-by: Serge Rielau <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit daa29f6) Signed-off-by: Wenchen Fan <[email protected]>

dongjoon-hyun · 2025-11-13T04:30:10Z

Hi, @srielau and @cloud-fan .

Is this a mistake? Or, did you want to make a huge followup? At the first glance, the JIRA Issue title looks weird to me.

$ git log --oneline | grep SPARK-53573
daa29f6ede2 [SPARK-53573][SQL] IDENTIFIER everywhere
4cf7772a84f [SPARK-53573][SQL] Allow coalescing string literals everywhere
983d384222d [SPARK-53573][SQL] Use Pre-processor for generalized parameter marker handling

dongjoon-hyun · 2025-11-13T04:31:06Z

Especially, this PR has 8k lines. It's a little hard for a PR of 8k lines to be a follow-up.

srielau · 2025-11-13T04:46:44Z

Hi, @srielau and @cloud-fan .

Is this a mistake? Or, did you want to make a huge followup? At the first glance, the JIRA Issue title looks weird to me.
$ git log --oneline | grep SPARK-53573
daa29f6ede2 [SPARK-53573][SQL] IDENTIFIER everywhere
4cf7772a84f [SPARK-53573][SQL] Allow coalescing string literals everywhere
983d384222d [SPARK-53573][SQL] Use Pre-processor for generalized parameter marker handling

This was intentional, albeit perhaps improper procedure.
These three PR's go together. The goal is to support parameterizing literals and identifiers everywhere.

Allow Parameters everywhere by moving substitution to a pre-parser
Expands string literal coalescing from constant to basic string literals
Fold IDENTIFIER of string literals during parsing
=> We can now have
spark.sql("SELECT 1 AS IDENTIFIER('C' :colordinal)"), Map("colordinal" -> "1"))
and
spark.sql("CREATE TABLE ... LOCATION :root '/somedir'", Map("root" -> "/volA")

I will work with @cloud-fan to structure it into subtasks

dongjoon-hyun · 2025-11-13T04:49:50Z

sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala

 * @see
 *   [[org.apache.spark.sql.catalyst.parser.AstBuilder]] for the full SQL statement parser
+ *
+ * ==CRITICAL: Extracting Identifier Names==


Does this CRITICAL: mean line 72 ~ 73?

'''DO NOT use ctx.getText() or ctx.identifier.getText()''' directly! These methods do not

handle the IDENTIFIER('literal') syntax and will cause incorrect behavior.

Yes. ideally I would have liked to block or override getText() but have not found a way to do so.

Thank you. Got it~

dongjoon-hyun · 2025-11-13T04:53:09Z

sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala

+ * Example:
+ * {{{
+ *   // WRONG - does not handle IDENTIFIER('literal'):
+ *   val name = ctx.identifier.getText


For my understanding, is this always wrong? What about the currently remaining (existing) code in AstBuilder.scala and SparkSqlParser.scala like the following?

$ git grep ctx.identifier.getText sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala: * '''DO NOT use ctx.getText() or ctx.identifier.getText()''' directly! These methods do not sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala: * val name = ctx.identifier.getText sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala: val collationName = ctx.identifier.getText sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala: lazy val name: String = ctx.identifier.getText sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala: lazy val name: String = ctx.identifier.getText sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala: ctx.identifier.getText.toLowerCase(Locale.ROOT) != "noscan") { sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala: ctx.identifier.getText.toLowerCase(Locale.ROOT) != "noscan") { sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala: val indexName = ctx.identifier.getText sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala: ctx.identifier.getText.toLowerCase(Locale.ROOT) match { sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala: ctx.identifier.getText.toLowerCase(Locale.ROOT) match {

yes! These slipped through the cracks. Must have fat-fingered my own grep to miss out on those.
I'll create a follow up.

github-actions bot added the SQL label Oct 28, 2025

dongjoon-hyun marked this pull request as draft October 28, 2025 18:26

srielau force-pushed the identifier-lite branch from e059de9 to b1b4bba Compare October 28, 2025 18:42

IDENTIFIER everywhere

4cbcebe

srielau force-pushed the identifier-lite branch from b1b4bba to 4cbcebe Compare October 28, 2025 19:09

srielau added 15 commits October 28, 2025 15:26

fix testcases

7b0da84

Merge branch 'master' into identifier-lite

d75a9d5

Merge branch 'master' into identifier-lite

6b3f044

Lots of fixes

c3a12e0

More fixes

2ee8073

Introduce SQLCOnf

ebe421a

Fix SQL Scripting

87cffc8

Fix testcases, linting

e10648c

Code review

193f946

Fix more testcases, some rework

0567456

Merge branch 'master' into identifier-lite

70833cc

Fix flaky test

582ab51

Delete temporray files

b9f46a3

remove unecessary testcases from ParametersSuite

733f874

move imports to head

srielau marked this pull request as ready for review November 6, 2025 05:15

srielau changed the title ~~[DRAFT] [SPARK-53573][SQL] IDENTIFIER everywhere~~ [SPARK-53573][SQL] IDENTIFIER everywhere Nov 6, 2025

srielau added 3 commits November 6, 2025 07:44

Make identifier-clause.sql less flaky

c43af8f

Simplify code, style fixes in error messages

e24a0ed

Fix testcases

9b2454a

dtenedor reviewed Nov 6, 2025

View reviewed changes

cloud-fan reviewed Nov 6, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

srielau added 3 commits November 6, 2025 13:14

Introduce simpleIdentifier

608e1ea

Fix build

9715ccb

Address comment by Wenchen

774eb2b

srielau added 3 commits November 11, 2025 09:09

Still fixing compoile errors

3593f5a

simplify

6f3e060

Fix sql scripting regression

ff191f0